[charts-pro] Handle edge case in export image#21190
Conversation
bernardobelchior
added
type: bug
bernardobelchior
force-pushed
the
export-image-edge-case
branch
from
d9e08ef to
eae380c
Compare
bernardobelchior
force-pushed
the
export-image-edge-case
branch
from
eae380c to
1aed049
Compare
| doc.body.removeChild(iframe); | ||
| throw new Error( | ||
| `MUI X Charts: Cannot export an image with zero width or height. Width: ${canvas.width}px. Height: ${canvas.height}px.\n` + | ||
| 'If this happens, please open an issue at github.com/mui/mui-x with a reproduction of the problem.', |
There was a problem hiding this comment.
This isn't necessarily our problem, but it's hard for us to ask the user to debug it, so I've suggested they open an issue. If there's a better solution let me know
There was a problem hiding this comment.
We could have a "debug" mode that doesn't clear the iframe after so users can debug themselves.
There was a problem hiding this comment.
They can technically use onBeforeExport to do that by inserting a debugger statement in that callback. What we can do is maybe explain that in the docs?
bernardobelchior
requested review from
JCQuintas and
alexfauquette
as code owners
|
Deploy preview: https://deploy-preview-21190--material-ui-x.netlify.app/ Bundle size report
|
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes |
| `MUI X Charts: Cannot export an image with zero width or height. Width: ${canvas.width}px. Height: ${canvas.height}px.\n` + | ||
| 'If this happens, please open an issue at github.com/mui/mui-x with a reproduction of the problem.', |
There was a problem hiding this comment.
Not sure about what we could do from those issues.
Either it's a simple issue on their codebase styling they could fix
Or it's hard to fix and so they will open an issue
| `MUI X Charts: Cannot export an image with zero width or height. Width: ${canvas.width}px. Height: ${canvas.height}px.\n` + | |
| 'If this happens, please open an issue at github.com/mui/mui-x with a reproduction of the problem.', | |
| `MUI X Charts: Cannot export an image with zero width or height (width: ${canvas.width}px, height: ${canvas.height}px).` |
Maybe adding an if(process.env.NODE_ENV !== 'production') to avoid throwing an error in production.
There was a problem hiding this comment.
Either it's a simple issue on their codebase styling they could fix
I don't think it'll be clear what to fix. I spent some time debugging to understand the root cause.
Or it's hard to fix and so they will open an issue
Yeah, that's why I'm suggesting they open an issue. That will probably help us understand more cases where this might be happening so we can preventively fix them from our side (e.g., by applying default styles).
Maybe I can only show the message "If this happens, please open an issue [...]" in development to avoid showing that to end users.
Maybe adding an if(process.env.NODE_ENV !== 'production') to avoid throwing an error in production.
What's your concern with an error? I think it's better to show an error if something goes wrong, otherwise people won't know. If you have something like Sentry to monitor errors, then these will be caught. If we just hide the error then developers won't know their users are facing problems in their app
There was a problem hiding this comment.
I don't think it'll be clear what to fix. I spent some time debugging to understand the root cause.
It will not be easier for us except if they manage to do a reproduction. And if they manage do do one, they basically solved their issue
What's your concern with an error?
From me you have two way to communicate about errors with devs:
console.errorinforms the devs that an error occurred but we mitigate itError()something went terrible and we can't do anything about that
I tend to prefer the first one because the second one without clean error boundaries can lead to some disturbing UI.
But not a big deal especially for pro users
There was a problem hiding this comment.
It will not be easier for us except if they manage to do a reproduction. And if they manage do do one, they basically solved their issue
That is true. I don't feel too strongly about it, so I'm ok with removing that section of the message.
I tend to prefer the first one because the second one without clean error boundaries can lead to some disturbing UI.
Actually, just realized we're catching all the error in useChartProExport and logging them as an error, so no errors escalate.
|
Cherry-pick PRs will be created targeting branches: v8.x |
4 participants
When the iframe's body style is set to certain values (e.g.,
display: 'flex'), the body won't have an intrinsic size, causing the resulting canvas to have a height of 0px, which causestoBlobto return null.You can reproduce this issue in our docs as follows:
Screen.Recording.2026-01-30.at.17.45.34.mov